Grand Central Dispatch #5130

pepyakin · 2020-03-04T17:51:39Z

Related to #5032

tomusdrw

Looks reasonable!

primitives/runtime/src/testing.rs

frame/executive/src/lib.rs

kianenigma

Would be interesting to see how this glues to the rest by

putting the docs there for the main trait
finishing bode/runtime/src/lib.rs

bin/node/runtime/src/lib.rs

athei · 2020-03-14T11:36:56Z

Rebased this PR onto the current master and fixed the test build. This was quiet a lot of work because the additional Dispatchabletrait bound that was added to frame_system::Trait::Call broke almost every test build.

The test are successful with the exception of the ui test which apparently tests against compiler output. Have to look into that I guess.

pepyakin · 2020-03-14T12:04:01Z

That's good news! I think ui tests could be fixed by merging in master again.

athei · 2020-03-15T13:01:53Z

That's good news! I think ui tests could be fixed by merging in master again.

It was just the missing documentation warning that made the UI test fail. Fixed that now. CI is finally happy and we are in sync with the current master. The exception is the check-runtime CI which we should ignore.

athei · 2020-03-16T08:29:48Z

Would be interesting to see how this glues to the rest by
* putting the docs there for the main trait

* finishing bode/runtime/src/lib.rs

I added some documentation for the trait. Regarding your second point. Can you elaborate what is missing from that file?

athei · 2020-03-16T13:22:37Z

This PR is now ready for review. I think I addressed all comments and now are patiently waiting for further comments / review.

bin/node-template/pallets/template/src/mock.rs

frame/collective/src/lib.rs

tomusdrw · 2020-03-16T15:50:45Z

frame/contracts/src/lib.rs

 	type TrieIdGenerator: TrieIdGenerator<Self::AccountId>;

+	/// The means of dispatching the calls to the runtime.
+	type Dispatcher: Dispatcher<<Self as Trait>::Call>;


Why should we always choose a parametrized dispatcher vs just using MainDispatcher coming from system?

See the discussion there.

TL;DR: I started it without thinking too much (since we doo the same with all other things) and then I found some benefits, namely using that as an escape hatch. I.e. the chain author can implement their own dispatcher that e.g. filters some calls / does additional bookkeeping and then delegates control to the system module. @athei 's idea was to actually introduce MainDispatcher to avoid hard-coding frame_system::Module directly but to just delegate to frame_system::Trait::MainDispatcher.

The reasoning is to give the runtime implementer the opportunity to customize the dispatch process for some Module. With the rewrite this is much safer in a way that dispatchers cannot manually call Dispatchable::dispatch but must specify a RootDispatcher in their implementation.

test-utils/runtime/src/lib.rs

primitives/runtime/src/generic/checked_extrinsic.rs

primitives/runtime/src/testing.rs

athei · 2020-03-17T08:48:09Z

Resolved conflicts with master and fixed and incorporated the suggestions from the comments. Should be good for merge soon (praying to CI god).

tomusdrw · 2020-03-17T09:38:58Z

bin/node/runtime/src/lib.rs

+	frame_system::ChainContext<Runtime>,
+	Runtime,
+	AllModules,
+	(),


Are there use cases for customizing the top-level (transaction-level) dispatcher?

If not, I'd remove this.

If yes, I'd rather see an enum DefaultDispatcher {} type that implements the dispatcher instead of (). While () works for associated types, here it lacks the context of what we are actually setting (You have to go to generic parameters and hope it has a nice readable name there).

There might be cases where someone want to provide another RootDispatcher. This is a far less intrusive change for a runtime implementer than modifying the frame_system which is the default RootDispatcher. Therefore I argue that it provides some benefit.

That said, I removed this parameter because we already specify the RootDispatcher through the frame_system::trait::RootDispatcher. No need for a redundant parameter.

bin/node-template/runtime/src/lib.rs

frame/collective/src/lib.rs

gavofyork · 2020-03-17T11:14:05Z

frame/system/src/lib.rs

+	/// important to delegate to the `RootDispatcher`. There might be use cases where the
+	/// delegation is unwanted and the custom dispatcher dispatches the `Dispatchable`
+	/// itself. It is important to understand that by doing this the dispatch becomes
+	/// invisible to the rest of the runtime machinery which relies on the `RootDispatcher`


Anytime there is something that is "important to understand" written in docs then I shivers down my spine.

This "API" (and I would use the term generously here, as it's far from codified) is dangerous.

I agree with you. I assumed that we need a way to enforce the correct usage which triggered this discussion. Bottom line was that the soft API or "escape hatch" was intentional. You are rightfully unhappy about it.

primitives/runtime/src/generic/checked_extrinsic.rs

gavofyork

Not at all happy about this PR. It introduces needless constraints and soft APIs that will ultimately lead to mishap.

I'd like to see a solid argument as to why needed changes for the contract-weight stuff cannot be kept isolated in the contracts module.

athei · 2020-03-17T11:37:39Z

Not at all happy about this PR. It introduces needless constraints and soft APIs that will ultimately lead to mishap.

I agree. We must do better.

I'd like to see a solid argument as to why needed changes for the contract-weight stuff cannot be kept isolated in the contracts module.

It is not only about contract-weight. As far as I understand it we want to be able to refund weight for every dispatch. For example a Balance::transfer which targets an existing account should be able to refund the weight that was reserved for the account creation.

athei · 2020-03-17T13:28:08Z

In response to the feedback we received I am planning the following larger changes apart from the minor issues that might or might not be resolved in the process.

Remove the Dispatchable trait bound from pallet_system::Trait::Call as adding it is way to restrictive with regard to testing
Redo the customization options for Runtime implementers so that it constitutes a hard to misuse API as it should be
Prevent the direct call of a Dispatchable without a a Dispatcher

kianenigma · 2020-03-17T18:36:45Z

I'd like to see a solid argument as to why needed changes for the contract-weight stuff cannot be kept isolated in the contracts module.

It is not only about contract-weight. As far as I understand it we want to be able to refund weight for every dispatch. For example a Balance::transfer which targets an existing account should be able to refund the weight that was reserved for the account creation.

Regardless if this approach, just emphasising that this was about more than just the contracts module and I think a weight refund in the long term is a good feature to have (through any means -- central dispatch was the best that we and @pepyakin came up with in a call. We can do better).

We might not use it at the time being since in Polkadot we want to stick to a worse case scenario. But a general substrate chain might want to be picky and have some means of refunding weight.

an example that we might want to consider refund even in polkadot is the phragmen submission transactions. If it goes through the hot path of replacing a solution, it takes a lot of time and it should rightfully take a whole lot of block's weight. Otherwise, it would be quite a waste to take all of that weight when we, in some cases, almost immediately reject the solution. Ideally the fee should still be taken but the weight should be refunded to the system module.

bkchr · 2020-03-18T07:46:47Z

TBH, I don't understand the purpose of this pr. Could someone sketch how that will help refunding weights?

athei · 2020-03-18T08:01:50Z

TBH, I don't understand the purpose of this pr. Could someone sketch how that will help refunding weights?

Weight refund will be done on a per dispatch basis as described here.

In order to do that we decided that going through a central instance that tracks the refunded gas per dispatch and deals with nested dispatches would be the best approach. #5032 already implements this central location as a dispatch_call method on the system pallet module.

The problem there is that this approach is purely advisory. Modules have to opt in by calling this new function instead of calling Dispatchable::dispatch. Because that approach is fragile we establish a mandatory weight tracking with this PR by forcing all dispatches through a Dispatcher. The mentioned #5032 will then be changed to integrate the weight refund into the Dispatcher.

bkchr · 2020-03-18T08:49:49Z

Hmm, so the problem you try to solve here isn't really solved IMHO.

In the default case where we don't have a wrapped call, we don't require this pr.

For the other case, where we have a wrapped call this weight refund doesn't make really sense. Let me explain why. For example think about a democracy proposal. After the proposal is accepted and we reached the point it should be applied, we will dispatch it. The proposal is applied in on_initialize, while on_initialize has some weight, it is probably way less than the weight of the actual call that will be dispatched as proposal. So, in your call you will refund some weight that wasn't taken for your call.

What we really want is probably an improved SignedExtension that is aware of dispatch and post_dispatch, but stuff like TransactionPayment should also not kick in for calls that are being dispatched from another module. I also already thought about making SignedExtension aware of wrapped calls, but didn't yet come to a proper solution. I'm open to discuss about this.

athei · 2020-03-18T09:06:03Z

Hmm, so the problem you try to solve here isn't really solved IMHO.

Yes. As I already stated the implementation is lacking. Working on it right now.

For the other case, where we have a wrapped call this weight refund doesn't make really sense. Let me explain why. For example think about a democracy proposal. After the proposal is accepted and we reached the point it should be applied, we will dispatch it. The proposal is applied in on_initialize, while on_initialize has some weight, it is probably way less than the weight of the actual call that will be dispatched as proposal. So, in your call you will refund some weight that wasn't taken for your call.

No. It does not work as you describe. The caller of the nested or wrapped call gets the unspent gas as return value and is responsible for handling it. It is all written down in the notion page I linked.

In order for that to work the dispatching call (here it is on_initialize) must take the dispatchees weight into consideration like sudo does:

#[weight = FunctionOf(
    |args: (&<T::Lookup as StaticLookup>::Source, &Box<<T as Trait>::Call>,)| {
        args.1.get_dispatch_info().weight + 10_000
    },
    |args: (&<T::Lookup as StaticLookup>::Source, &Box<<T as Trait>::Call>,)| {
        args.1.get_dispatch_info().class
    },
    true
)]

Then when the caller gets the unspent weight from the call it dispatched and can decide to refund it. Or it does not dispatch at all and refund all the weight that was reserved for the wrapped call.

What we really want is probably an improved SignedExtension that is aware of dispatch and post_dispatch, but stuff like TransactionPayment should also not kick in for calls that are being dispatched from another module. I also already thought about making SignedExtension aware of wrapped calls, but didn't yet come to a proper solution. I'm open to discuss about this.

The other PR I linked takes care of that. It changes the SignedExtensions to be aware of weight refund. It will ask the Dispatcher for the unspent weight of the Extrinsic. Wrapped calls are handled by their caller as described above. SignedExtensions therefore only deal with the Extrinsic dispatched call. Not what is wrapped inside of it.

This introduce a `Dispatcher` trait that is used to dispatch all calls instead of directly calling `Dispatchable::dispatch`. This is needed in order to do centralistic bookeeping for all calls like for example weight refunds. Co-Authored-By: pepyakin <[email protected]>

This adds an argument to the above function wich is only constructable by the `sp_runtime::traits` module.

gavofyork · 2020-03-23T12:27:11Z

It sounds like the only API change required is a minor one that just ensures dispatchables (and on_initialize) return the weight consumed.

we decided that going through a central instance

I do not want to see any new "central instances" of anything.

athei · 2020-03-23T16:43:04Z

It sounds like the only API change required is a minor one that just ensures dispatchables (and on_initialize) return the weight consumed.

we decided that going through a central instance

I do not want to see any new "central instances" of anything.

That is another approach to the weight refund task. One could modify the Dispatchable and the macros that generate its implementations so that it does the weight accounting for every call. This is clearly a more elegant approach.

It comes with its own drawbacks, though.

It would introduce a frame concept (weight) to Dispatchable that lives in primitives and should therefore be runtime agnostic. This can be worked around by making the return type of Dispatchable generic, I guess. So it is not necessarily weight but any additional data that a Dispatchable can deliver in addition to the DispatchResult.
What about frame chains that do not want to use weight refund? Generating the refund logic in macros makes it more difficult to make the concept pluggable (correct me if I am wrong). In its current state of this PR this would be just a matter of setting frame_system::trait::RootDispatcher = SimpleDispatcher. I did not do much thinking in this direction, yet. There might be a good solution for this. It might require an even larger change set than this PR, though.

@gavofyork Is this a direction you would rather see the weight refund stuff going?

kianenigma · 2020-03-24T07:05:26Z

I think in the grand scheme of things, it is easier to think of it this way:

Instead of asking dispatchables to return the used weight (or signal it via some function or any other means), it should be assumed that they consume all of it unless if they return some value. So, if I have understood it correctly, I agree with

What about frame chains that do not want to use weight refund? Generating the refund logic in macros makes it more difficult to make the concept pluggable (correct me if I am wrong)

kianenigma · 2020-03-24T07:16:59Z

frame/democracy/src/lib.rs

-				let ok = proposal.dispatch(frame_system::RawOrigin::Root.into()).is_ok();
+				let ok = T::Dispatcher::dispatch(
+					proposal,
+					frame_system::RawOrigin::Root.into()


Suggested change

frame_system::RawOrigin::Root.into()

frame_system::RawOrigin::Root.into(),

kianenigma · 2020-03-24T07:17:08Z

frame/democracy/src/lib.rs

 		Ok(())
 	}
-}
+}


Suggested change

}

}

kianenigma · 2020-03-24T07:18:58Z

frame/executive/src/lib.rs

-		let r = Applyable::apply::<UnsignedValidator>(xt, dispatch_info, encoded_len)?;
+		let r = Applyable::apply::<UnsignedValidator, System::RootDispatcher>(xt,
+			dispatch_info,
+			encoded_len


Suggested change

encoded_len

encoded_len,

kianenigma · 2020-03-24T07:21:07Z

frame/system/src/lib.rs

+	/// For `Dispatchable` calls it is usually the responsiblity of the frame_system
+	/// module to function as the root `RootDispatcher`. In order to customize the
+	/// dispatch process for a specific module the `Dispatcher` trait can be implemented
+	/// and supplied to to the respective `Trait`.


Suggested change

/// and supplied to to the respective `Trait`.

/// and supplied to the respective `Trait`.

kianenigma · 2020-03-24T07:23:52Z

primitives/runtime/src/lib.rs

 	}
 }

+/// This type describes the result of a module function call which is called through


nit: I'd put these guys next to SimpleDispatcher.

Or not. There's a lot more dispatch-related and sadly the ordering is a bit confused. nvmd.

kianenigma · 2020-03-24T07:27:38Z

primitives/runtime/src/traits.rs

+/// In order to provide a custom dispatcher this type should be implemented and then
+/// and then used by calling `dispatch` with a `Dispatchable`.


Suggested change

/// In order to provide a custom dispatcher this type should be implemented and then

/// and then used by calling `dispatch` with a `Dispatchable`.

/// In order to provide a custom dispatcher this type should be implemented

/// and then used by calling `dispatch` with a `Dispatchable`.

kianenigma · 2020-03-24T07:31:22Z

primitives/runtime/src/traits.rs

+	/// Use `Self::raw_dspatch` in order to do the actual dispatch as direct call
+	/// of `Dispatchable::dispatch` is prevent by a token. This function should be called


Suggested change

/// Use `Self::raw_dspatch` in order to do the actual dispatch as direct call

/// of `Dispatchable::dispatch` is prevent by a token. This function should be called

/// Use `Self::raw_dspatch` in order to do the actual dispatch as direct call

/// of `Dispatchable::dispatch`. ???? is prevent by a token. This function should be called

athei · 2020-04-06T07:01:06Z

Closed in favor of #5458

pepyakin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 4, 2020

tomusdrw reviewed Mar 5, 2020

View reviewed changes

primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved

frame/executive/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Mar 5, 2020

View reviewed changes

athei self-assigned this Mar 12, 2020

athei force-pushed the ser-grand-central-dispatch branch from d9f689d to cfe572b Compare March 13, 2020 13:01

paritytech-ci added the B2-breaksapi label Mar 13, 2020

athei reviewed Mar 14, 2020

View reviewed changes

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved

athei force-pushed the ser-grand-central-dispatch branch from 290d119 to a0cb883 Compare March 15, 2020 11:10

athei marked this pull request as ready for review March 16, 2020 13:20

athei requested review from Demi-Marie, andresilva, gui1117 and mxinden as code owners March 16, 2020 13:20

athei added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 16, 2020

athei requested review from kianenigma and tomusdrw March 16, 2020 13:21

athei changed the title ~~Grand Central Dispatch RFC~~ Grand Central Dispatch Mar 16, 2020

athei mentioned this pull request Mar 16, 2020

Weight Refunds mechanism RFC #5032

Closed

tomusdrw approved these changes Mar 16, 2020

View reviewed changes

athei force-pushed the ser-grand-central-dispatch branch from b032480 to 6f1fabf Compare March 17, 2020 08:44

tomusdrw approved these changes Mar 17, 2020

View reviewed changes

kianenigma reviewed Mar 17, 2020

View reviewed changes

bin/node-template/runtime/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Mar 17, 2020

View reviewed changes

frame/collective/src/lib.rs Show resolved Hide resolved

gavofyork reviewed Mar 17, 2020

View reviewed changes

primitives/runtime/src/generic/checked_extrinsic.rs Outdated Show resolved Hide resolved

gavofyork suggested changes Mar 17, 2020

View reviewed changes

gavofyork added A4-gotissues and removed A0-please_review Pull request needs code review. labels Mar 17, 2020

athei and others added 2 commits March 23, 2020 12:34

Prevent Dispatchable::dispatch to be called without a Dispatcher

a6a00c3

This adds an argument to the above function wich is only constructable by the `sp_runtime::traits` module.

athei force-pushed the ser-grand-central-dispatch branch from 6f1fabf to a6a00c3 Compare March 23, 2020 11:44

tomusdrw mentioned this pull request Mar 23, 2020

Implement ValidateUnsigned as SignedExtension #5006

Closed

kianenigma reviewed Mar 24, 2020

View reviewed changes

frame/democracy/src/lib.rs

Ok(())

}

}

}

Copy link

Contributor

kianenigma Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

}

}

kianenigma reviewed Mar 24, 2020

View reviewed changes

athei mentioned this pull request Mar 30, 2020

Make Dispatchable return the actual weight consumed #5458

Merged

2 tasks

athei closed this Apr 6, 2020

bkchr deleted the ser-grand-central-dispatch branch April 6, 2020 08:57

	frame_system::RawOrigin::Root.into()
	frame_system::RawOrigin::Root.into(),

	/// and supplied to to the respective `Trait`.
	/// and supplied to the respective `Trait`.

		/// In order to provide a custom dispatcher this type should be implemented and then
		/// and then used by calling `dispatch` with a `Dispatchable`.

		/// Use `Self::raw_dspatch` in order to do the actual dispatch as direct call
		/// of `Dispatchable::dispatch` is prevent by a token. This function should be called

Grand Central Dispatch #5130

Grand Central Dispatch #5130

Uh oh!

Conversation

pepyakin commented Mar 4, 2020

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kianenigma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

athei commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepyakin commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Mar 16, 2020

Uh oh!

athei commented Mar 16, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athei commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athei Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

athei commented Mar 17, 2020

Uh oh!

athei commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kianenigma commented Mar 17, 2020

Uh oh!

bkchr commented Mar 18, 2020

Uh oh!

athei commented Mar 18, 2020

Uh oh!

bkchr commented Mar 18, 2020

Uh oh!

athei commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavofyork commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Mar 23, 2020

Uh oh!

kianenigma commented Mar 24, 2020

kianenigma left a comment •

edited

Loading

athei commented Mar 14, 2020 •

edited

Loading

pepyakin commented Mar 14, 2020 •

edited

Loading

athei commented Mar 15, 2020 •

edited

Loading

athei Mar 17, 2020 •

edited

Loading

athei commented Mar 17, 2020 •

edited

Loading

athei commented Mar 18, 2020 •

edited

Loading

gavofyork commented Mar 23, 2020 •

edited

Loading